Skip to content

Enlighten status printer fixes#91

Open
AlexJones0 wants to merge 3 commits intolowRISC:masterfrom
AlexJones0:status_printer_fixes
Open

Enlighten status printer fixes#91
AlexJones0 wants to merge 3 commits intolowRISC:masterfrom
AlexJones0:status_printer_fixes

Conversation

@AlexJones0
Copy link
Contributor

A couple of small bug fixes / cleanups to the EnlightenStatusPrinter - see the commit messages for more details. There is definitely potential for refactoring the help message logic but the change is kept intentionally small for now to focus on the bug fix, and because larger-scale refactors/rewrites may come later.

Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks nice to me (but I'd suggest slightly tweaking the print_header rejig)

Copy link
Collaborator

@machshev machshev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks @AlexJones0

Keep this as the default value and move it to a named constant for now
to avoid repetition. This abstraction likely needs more work and this
can be moved to a different location (the status table in general is
quite entangled with the JobStatus), but that refactoring should be
handled independent of this commit, which is just applying a fix.

Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
If the EnlightenStatusPrinter is initialized and then `.exit()` is
called without ever first calling `.print_header()`, then the `.exit()`
method will attempt to call the `.close()` method on `None`, giving an
`AttributeError`. Add a presence check to fix this.

Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Enlighten registers logic to (at least partially) reset the terminal
using the `atexit` library and clean up, but we should explicitly close
the Enlighten manager when we cleanup/exit the status printer to make
sure that the terminal is fully restored to its normal working state.

Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants